Code Review: Godot Static Checks Workflow

Summary

This PR adds a GitHub Actions workflow for Godot-specific static checks including GDScript formatting, linting, and spell checking. Overall, this is a solid addition that will help maintain code quality as the project develops.

✅ Strengths

1. Appropriate tooling: Using godot-gdscript-toolkit is the right choice for GDScript linting and formatting
2. Efficient path filtering (.github/workflows/godot-checks.yml:7-9): Only runs when addons/** or scripts/** change, avoiding unnecessary workflow runs
3. Concurrency control (.github/workflows/godot-checks.yml:11-13): Good use of concurrency groups to cancel in-progress runs
4. Spell checking: Including codespell is a nice touch for catching typos

⚠️ Issues & Recommendations

1. Critical: Missing permissions block

All other workflows in this repo (checkov.yml, actionlint.yml, markdownlint.yml) explicitly declare permissions with a global permissions: {} and job-level granular permissions. This workflow should follow the same security pattern:

# global permissions
permissions: {}

jobs:
  static-checks:
    runs-on: ubuntu-latest
    permissions:
      contents: read  # for actions/checkout
    steps:
      # ...

This follows the principle of least privilege and matches the repository's established security standards.

2. Action pinning inconsistency

• .github/workflows/godot-checks.yml:20 uses actions/checkout@v6
• Other workflows use actions/checkout@v4 (checkov.yml:32, actionlint.yml:22, markdownlint.yml:18)

While v6 may be newer, consistency across the repo is valuable. Consider:

• Either update all workflows to v6 (separate PR), or
• Use v4 here to match existing patterns

3. Using branch instead of version tag

.github/workflows/godot-checks.yml:21 uses Scony/godot-gdscript-toolkit@master

This is a security and reproducibility concern:

• master branch can change at any time, potentially breaking your CI or introducing malicious code
• Best practice is to pin to a specific version tag or commit SHA
• Check if this action has version tags; if not, consider using a commit SHA

4. Directories don't exist yet

The workflow filters on addons/** and scripts/** paths, but these directories don't currently exist in the repository. While this is forward-looking, consider:

• Is this premature? Should you wait until you actually have GDScript files to check?
• Or is this intentional preparation for upcoming work?

If intentional, this is fine, but the workflow will simply skip on all current commits.

5. Spell check configuration concern

.github/workflows/godot-checks.yml:29-30: The ignore_words_list contains Manuel - this looks like it might be a proper name. Consider whether this belongs in a project-specific .codespellrc file instead of the workflow, for better maintainability.

Also, there's a trailing space after Manuel, in the ignore_words_list which might cause parsing issues.

6. No failure handling

Unlike checkov.yml which uses if: success() || failure() for certain steps, this workflow has no error handling strategy. Consider:

• Should formatting failures block PRs completely, or should they be warnings?
• Should spell check failures be blocking?

🔒 Security Considerations

• Missing explicit permissions (see issue 🎾 [readme] remove template-repo debris for a clean start #1 above)
• Using @master branch reference is a supply chain risk
• Overall: Medium security concern that should be addressed

📋 Test Coverage

This workflow itself has no tests, which is typical for GitHub Actions. However:

• The workflow will be tested by actionlint.yml
• Consider testing it manually on a branch with sample GDScript files

🎯 Recommendations Priority

1. HIGH: Add explicit permissions block
2. HIGH: Pin godot-gdscript-toolkit to a version/SHA instead of @master
3. MEDIUM: Align checkout action version with other workflows
4. LOW: Clean up ignore_words_list (remove trailing space after Manuel)
5. LOW: Consider moving spell check config to .codespellrc file

📝 Suggested Fix

Here's a suggested improved version addressing the high-priority issues:

name: Static Checks 📊
on:
  push:
    branches-ignore:
      - gh-pages
  pull_request:
    paths:
      - "addons/**"
      - "scripts/**"

# global permissions
permissions: {}

concurrency:
  group: ci-${{github.actor}}-${{github.head_ref || github.run_number}}-${{github.ref}}-static
  cancel-in-progress: true

jobs:
  static-checks:
    runs-on: ubuntu-latest
    permissions:
      contents: read  # for actions/checkout to fetch code
    steps:
      - name: Checkout
        uses: actions/checkout@v4
      - uses: Scony/godot-gdscript-toolkit@<version-tag-or-sha>
      - name: Formatting checks
        run: gdformat --diff .
      - name: Linting checks
        run: gdlint .
      - name: Spell checks via codespell
        uses: codespell-project/actions-codespell@v2
        with:
          skip: ./addons,*.po
          ignore_words_list: chello,doubleclick,Manuel,SectionIn

✨ Overall Assessment

This is a good addition that demonstrates forward-thinking about code quality. With the security fixes applied (permissions and pinning), this will be a solid workflow. The main concern is following the repository's established security patterns.

🤖 Generated by Claude Code Review

Code Review - PR #4: Add Godot-specific checks

Summary

This PR adds a GitHub Actions workflow for Godot-specific static checks including GDScript formatting, linting, and spell checking. Overall, the implementation is solid with good security practices (global permissions set to {}).

Issues & Recommendations

🔴 Critical Issues

1. Missing addons/ and scripts/ directories (.github/workflows/godot-checks.yml:7-9)

   • The workflow filters on addons/** and scripts/** paths, but these directories don't exist in the repository yet
   • This means the workflow will never run until Godot code is added
   • Recommendation: Either remove the path filters for now, or create placeholder directories with .gitkeep files

2. Inconsistent checkout action version (.github/workflows/godot-checks.yml:23)

   • Uses actions/checkout@v6 while other workflows use @v4 (see markdownlint.yml:18, actionlint.yml:22)
   • Recommendation: Standardize on @v4 for consistency, or update all workflows to @v6

🟡 Medium Priority Issues

3. Using @master for third-party action (.github/workflows/godot-checks.yml:24)

   • Scony/godot-gdscript-toolkit@master pins to a moving target, which is a security and stability risk
   • Best practice is to pin to specific commit SHA or version tag
   • Recommendation: Pin to a specific version: Scony/godot-gdscript-toolkit@<commit-sha> or use a release tag if available

4. Missing job-level permissions (.github/workflows/godot-checks.yml:19)

   • Global permissions are set to {} (good!), but the job doesn't declare contents: read
   • Other workflows (markdownlint.yml:16, actionlint.yml:20) follow this pattern
   • Recommendation: Add job-level permissions:

   static-checks:
     runs-on: ubuntu-latest
     permissions:
       contents: read

5. Trailing space in ignore_words_list (.github/workflows/godot-checks.yml:33)

   • ignore_words_list: chello,doubleclick,Manuel, SectionIn has a space after "Manuel,"
   • May cause unexpected behavior
   • Recommendation: Remove space: ignore_words_list: chello,doubleclick,Manuel,SectionIn

🟢 Minor Issues / Suggestions

6. Missing workflow documentation

   • No comments explaining what gdformat/gdlint do or why specific words are ignored
   • Recommendation: Add brief comments for future maintainers

7. Branch filter inconsistency

   • Uses branches-ignore while other workflows use branches (actionlint.yml:6-8)
   • Both are valid, but consistency improves maintainability
   • Recommendation: Consider using branches: ["main"] on push events to match other workflows

✅ What's Done Well

• Global permissions set to {} (addresses the "fix global GHA permissions" commit)
• Concurrency control with cancel-in-progress: true (efficient resource usage)
• Good separation of concerns (formatting, linting, spell checking as separate steps)
• Uses established tools (codespell, gdlint, gdformat)

Testing Recommendation

Since there are no .gd files yet, this workflow won't be triggered. Consider:

• Testing the workflow manually with workflow_dispatch trigger, or
• Adding sample Godot files to verify the workflow works as expected

Security Assessment

✅ No security concerns. The global permissions fix is correctly implemented.

Review provided by Claude Code

Code Review - PR #4

Summary

This PR adds Godot-specific CI checks for GDScript formatting, linting, and spell checking. The workflow is well-structured and follows repository conventions.

Positive Aspects

1. Security Best Practices ✅

   • Correctly implements global permissions: {} with no job-level overrides needed
   • Follows the security pattern established in other workflows (.github/workflows/checkov.yml:15, .github/workflows/markdownlint.yml:10)

2. Appropriate Triggers ✅

   • branches-ignore: gh-pages prevents unnecessary runs on documentation branches
   • Path filtering (addons/**, scripts/**) ensures the workflow only runs when relevant Godot files change
   • Runs on both push and pull_request for comprehensive coverage

3. Concurrency Control ✅

   • Proper concurrency group with cancel-in-progress: true prevents wasted CI resources
   • Unique group identifier prevents conflicts with other workflows

4. Clear Naming ✅

   • Workflow name "Godot Checks 📊" matches the filename godot-checks.yml (per commit 3)
   • Descriptive step names make the workflow easy to understand

Issues & Recommendations

🔴 Critical: Pinning Third-Party Action to Master Branch

Location: .github/workflows/godot-checks.yml:24

- uses: Scony/godot-gdscript-toolkit@master

Issue: Using @master is a security and stability risk. The master branch can change at any time, potentially:

• Introducing breaking changes that fail your CI
• Containing malicious code if the upstream repo is compromised
• Making builds non-reproducible

Recommendation: Pin to a specific commit SHA or version tag:

- uses: Scony/godot-gdscript-toolkit@<commit-sha>

Check the repository for available releases/tags and use the most recent stable version.

⚠️ Medium: Missing Job-Level Permissions

Location: .github/workflows/godot-checks.yml:18-20

While the workflow correctly sets global permissions: {}, the godot-checks job should explicitly declare the permissions it needs.

Current:

jobs:
  godot-checks:
    runs-on: ubuntu-latest

Recommended:

jobs:
  godot-checks:
    runs-on: ubuntu-latest
    permissions:
      contents: read  # for actions/checkout

This follows the pattern in markdownlint.yml:15-16 and makes permissions explicit.

⚠️ Medium: Checkout Action Version Mismatch

Location: .github/workflows/godot-checks.yml:23

- uses: actions/checkout@v6

Issue: Other workflows in the repository use @v4 (see checkov.yml:32, markdownlint.yml:18). While v6 may be intentional for future-proofing, this inconsistency should be addressed.

Recommendation: Either:

1. Use @v4 for consistency with existing workflows, OR
2. Update all workflows to @v6 in a separate PR

💡 Minor: Codespell Configuration

Location: .github/workflows/godot-checks.yml:32

skip: ./addons,*.po

Issue: The skip parameter uses ./addons (relative path with ./) which may behave inconsistently.

Recommendation: Use consistent path format:

skip: addons,*.po

Also, the ignore_words_list contains Manuel (capitalized), which seems like it might be a typo or project-specific name. Consider documenting why these specific words are ignored (either in the workflow comments or in a .codespellrc file).

💡 Minor: Path Filters May Be Too Restrictive

Location: .github/workflows/godot-checks.yml:7-9

paths:
  - "addons/**"
  - "scripts/**"

Question: Should the workflow also run when the workflow file itself changes? Consider adding:

paths:
  - "addons/**"
  - "scripts/**"
  - ".github/workflows/godot-checks.yml"

This ensures changes to the CI configuration are validated.

Test Coverage

Gap: The repository currently has no Godot files (no .gd files found, no addons/ or scripts/ directories exist yet). This means:

• The workflow cannot be tested until Godot code is added
• The path filters are untested
• The tools (gdformat, gdlint, codespell) haven't been validated against actual code

Recommendation: Consider adding a simple test fixture or waiting to merge this until there's actual Godot code to validate against.

Overall Assessment

Status: Approve with minor changes recommended

The workflow is well-designed and follows repository conventions. The critical issue (unpinned @master action) should be fixed before merging. The other suggestions improve consistency and security but aren't blockers.

Priority Fixes:

1. Pin Scony/godot-gdscript-toolkit to a specific version/SHA
2. Add explicit permissions: contents: read to the job
3. Align actions/checkout version with other workflows

Review generated by Claude Code